[KEP-0009] feat: add expression based assertions #576

kumar-mallikarjuna · 2024-11-23T08:45:22Z

What this PR does / why we need it:
This PR adds CEL-expression based assertions to TestAsserts. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details.

Fixes #562

porridge

Nice, but needs a few changes.

pkg/apis/testharness/v1beta1/test_types.go

pkg/test/utils/kubernetes.go

eddycharly · 2024-11-25T11:05:05Z

pkg/test/utils/kubernetes.go

+		variables[resourceRef.Id] = referencedResource.Object
+	}
+
+	env, err := cel.NewEnv()


Calling cel.NewEnv() may not be enough, you probably want to enable a couple of options/libs.

@eddycharly any hints on what might be useful? 🤔

pkg/test/utils/kubernetes.go

pkg/apis/testharness/v1beta1/expression.go

pkg/test/step.go

pkg/test/utils/kubernetes.go

pkg/test/utils/expression.go

pkg/test/utils/kubernetes.go

kumar-mallikarjuna · 2024-12-03T17:24:36Z

Thanks for the quick review @porridge . I'll add the tests tomorrow.

porridge

Thanks, this looks much better, but some more changes are needed. 🙏🏻
Please see inline.

pkg/test/step.go

pkg/test/expression_integration_test.go

pkg/expressions/cel.go

pkg/test/step.go

pkg/apis/testharness/v1beta1/expression.go

pkg/expressions/cel.go

pkg/test/expression_integration_test.go

porridge

Almost there!
Just a bunch of nitpicks for messages and identifier names, and one issue in the test..

pkg/expressions/cel.go

pkg/test/expression_integration_test.go

...gration_test_data/assert_expressions/check_expression_for_ephemeral_namespace/01-assert.yaml

...gration_test_data/assert_expressions/check_expression_for_ephemeral_namespace/00-create.yaml

porridge · 2024-12-19T12:26:41Z

pkg/test/utils/kubernetes.go

+		variables[resourceRef.Id] = referencedResource.Object
+	}
+
+	env, err := cel.NewEnv()


@eddycharly any hints on what might be useful? 🤔

porridge

A few more things.

pkg/test/expression_integration_test.go

pkg/expressions/cel.go

pkg/test/expression_integration_test.go

...gration_test_data/assert_expressions/check_expression_for_ephemeral_namespace/01-assert.yaml

This PR adds CEL-expression based assertions to `TestAsserts`. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details. Signed-off-by: Kumar Mallikarjuna <[email protected]>

Signed-off-by: Kumar Mallikarjuna <[email protected]>

…re present Signed-off-by: Kumar Mallikarjuna <[email protected]>

Signed-off-by: Kumar Mallikarjuna <[email protected]>

…pace Signed-off-by: Kumar Mallikarjuna <[email protected]>

Signed-off-by: Kumar Mallikarjuna <[email protected]>

porridge

LGTM now, just the debugging leftovers need to go :-)

pkg/test/step.go

pkg/test/expression_integration_test.go

Signed-off-by: Kumar Mallikarjuna <[email protected]>

eddycharly · 2025-01-08T20:32:25Z

Shouldn't you verify that the program return type is a boolean ?

Something like:

...
if !ast.OutputType().IsExactType(types.BoolType) ...
...

eddycharly · 2025-01-08T20:35:55Z

pkg/expressions/cel.go

+		return nil, fmt.Errorf("failed to load resource reference(s): %w", errors.Join(errs...))
+	}
+
+	env, err := cel.NewEnv()


Are you extending the env somewhere ? Looks like you will want to register a bunch of CEL libs to enable features.

eddycharly · 2025-01-08T20:42:17Z

pkg/expressions/cel.go

+	}
+
+	for _, resourceRef := range resourceRefs {
+		env, err = env.Extend(cel.Variable(resourceRef.Ref, cel.DynType))


Would be nice to use a type provider instead of DynType.

eddycharly · 2025-01-08T20:46:19Z

pkg/expressions/cel.go

+}
+
+func evaluateExpression(expr string,
+	programs map[string]cel.Program,


Why create a map to lookup the program corresponding to an expression ? 🤔

eddycharly · 2025-01-08T20:49:29Z

pkg/apis/testharness/v1beta1/test_types.go

@@ -157,6 +157,11 @@ type TestAssert struct {
 	Collectors []*TestCollector `json:"collectors,omitempty"`
 	// Commands is a set of commands to be run as assertions for the current step
 	Commands []TestAssertCommand `json:"commands,omitempty"`
+
+	ResourceRefs []TestResourceRef `json:"resourceRefs,omitempty"`


You should consider two TestResourceRef with the same id an error.

eddycharly · 2025-01-08T20:52:53Z

pkg/expressions/cel.go

+	var anyExprErrors, allExprErrors []error
+	for _, expr := range assertAny {
+		if err := evaluateExpression(expr.CELExpression, programs, variables); err != nil {
+			anyExprErrors = append(anyExprErrors, err)
+		}
+	}
+
+	for _, expr := range assertAll {
+		if err := evaluateExpression(expr.CELExpression, programs, variables); err != nil {
+			allExprErrors = append(allExprErrors, err)
+		}
+	}
+
+	if len(assertAny) != 0 && len(anyExprErrors) == len(assertAny) {
+		errs = append(errs, fmt.Errorf("no expression evaluated to true: %w", errors.Join(anyExprErrors...)))
+	}
+
+	if len(allExprErrors) > 0 {
+		errs = append(errs, fmt.Errorf("not all assertAll expressions evaluated to true: %w", errors.Join(allExprErrors...)))
+	}
+
+	return errs


This would make sense to return early here instead of evaluating all expressions before making a decision.

eddycharly · 2025-01-08T20:54:17Z

pkg/test/step.go

+	variables := make(map[string]interface{})
+	for _, resourceRef := range s.Assert.ResourceRefs {
+		if resourceRef.Namespace == "" {
+			resourceRef.Namespace = namespace
+		}
+		namespacedName, referencedResource := resourceRef.BuildResourceReference()
+		if err := client.Get(context.TODO(), namespacedName, referencedResource); err != nil {
+			return []error{fmt.Errorf("failed to get referenced resource '%v': %w", namespacedName, err)}
+		}
+
+		variables[resourceRef.Ref] = referencedResource.Object
+	}


This should be lazy loaded.

porridge requested changes Nov 25, 2024

View reviewed changes

eddycharly reviewed Nov 25, 2024

View reviewed changes

pkg/test/utils/kubernetes.go Outdated Show resolved Hide resolved

kumar-mallikarjuna force-pushed the KEP-0009 branch 2 times, most recently from 0fe748e to a85752a Compare December 3, 2024 07:56

kumar-mallikarjuna requested a review from porridge December 3, 2024 09:33

porridge requested changes Dec 3, 2024

View reviewed changes

kumar-mallikarjuna mentioned this pull request Dec 4, 2024

Refactor pkg/test/utils/kubernetes.go #589

Closed

kumar-mallikarjuna marked this pull request as ready for review December 4, 2024 17:47

kumar-mallikarjuna requested a review from kensipe as a code owner December 4, 2024 17:47

kumar-mallikarjuna requested a review from porridge December 5, 2024 13:46

porridge requested changes Dec 10, 2024

View reviewed changes

kumar-mallikarjuna force-pushed the KEP-0009 branch from 7044f54 to a7122a6 Compare December 19, 2024 07:44

porridge requested changes Dec 19, 2024

View reviewed changes

kumar-mallikarjuna force-pushed the KEP-0009 branch from 1752619 to 32bcb9c Compare December 19, 2024 17:04

kumar-mallikarjuna requested review from eddycharly and porridge January 2, 2025 06:06

porridge requested changes Jan 2, 2025

View reviewed changes

kumar-mallikarjuna requested a review from porridge January 7, 2025 12:36

kumar-mallikarjuna added 10 commits January 7, 2025 18:09

[KEP-0009] feat: add expression based assertions

bd77825

This PR adds CEL-expression based assertions to `TestAsserts`. See https://github.com/kudobuilder/kuttl/blob/main/keps/0009-expression-based-assertions.md for more details. Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: rename Id->Ref and make linter happy

68c60e3

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: add validation for resourceRefs

9a94a6b

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: make assertion syntax consistent with the KEP

38819ef

Signed-off-by: Kumar Mallikarjuna <[email protected]>

refactor: rename Validate method for TestResourceRef

3a6b02e

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: pre-build environment and program for expressions

bd362f2

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: make linter happy and initialize Programs only if assertions a…

14ad7a8

…re present Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

6df2e32

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: move RunAssertExpressions() to pkg/expressions

aa6cb65

Signed-off-by: Kumar Mallikarjuna <[email protected]>

refactor: move CEL program loading to a dedicated function

832bf9e

Signed-off-by: Kumar Mallikarjuna <[email protected]>

kumar-mallikarjuna added 19 commits January 7, 2025 18:09

refactor: move program-loading to Step out of LoadPrograms()

e32b53e

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: add tests for TestResourceRef

3223ada

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: correct evaluation for assertAll

f08a27e

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: add integration tests for expressions

7cb27fc

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: make linter happy

3fcf018

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

c6c3e8a

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: change array reference for all-asserts

51e3deb

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: change signature for CheckAssertExpressions()

32330e9

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: use envtest in expression_integration_test.go

5e252f5

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: remove redundant definitions for integration test tools

56c1779

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: use testenv in expression_integration_test.go

2c58b63

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: run target expressions test within a Kuttl owned ephemeral names…

aa2fa55

…pace Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: remove redundant file and update go.mod

9e77060

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

f824fb9

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: update error messages

965b7eb

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: update error message

5173e6c

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: incorporate review comments

4f358b0

Signed-off-by: Kumar Mallikarjuna <[email protected]>

fix: update context creation

60b818d

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore: aggregate test steps into one

dd85a98

Signed-off-by: Kumar Mallikarjuna <[email protected]>

kumar-mallikarjuna force-pushed the KEP-0009 branch from dd2131a to dd85a98 Compare January 7, 2025 12:40

porridge requested changes Jan 8, 2025

View reviewed changes

pkg/test/step.go Outdated Show resolved Hide resolved

pkg/test/expression_integration_test.go Outdated Show resolved Hide resolved

kumar-mallikarjuna added 2 commits January 8, 2025 22:21

chore: remove debug statements

d510656

Signed-off-by: Kumar Mallikarjuna <[email protected]>

chore(tests): remove prefixes for expression integration test steps

7c164ba

Signed-off-by: Kumar Mallikarjuna <[email protected]>

kumar-mallikarjuna requested a review from porridge January 8, 2025 17:17

eddycharly reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KEP-0009] feat: add expression based assertions #576

[KEP-0009] feat: add expression based assertions #576

kumar-mallikarjuna commented Nov 23, 2024

porridge left a comment

eddycharly Nov 25, 2024

porridge Dec 19, 2024

kumar-mallikarjuna commented Dec 3, 2024

porridge left a comment

porridge left a comment

porridge Dec 19, 2024

porridge left a comment

porridge left a comment

eddycharly commented Jan 8, 2025 •

edited

Loading

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

eddycharly Jan 8, 2025

[KEP-0009] feat: add expression based assertions #576

Are you sure you want to change the base?

[KEP-0009] feat: add expression based assertions #576

Conversation

kumar-mallikarjuna commented Nov 23, 2024

porridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar-mallikarjuna commented Dec 3, 2024

porridge left a comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

eddycharly commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddycharly commented Jan 8, 2025 •

edited

Loading